fix: Untrace entire request #968
Conversation
robertlaurin
force-pushed
the
rack-event-untraced
branch
from
c044a32
to
e7f2e48
Compare
robertlaurin
force-pushed
the
rack-event-untraced
branch
from
e7f2e48
to
fe56a27
Compare
robertlaurin
requested review from
fbogsany,
mwear,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle and
xuan-cao-swi
as code owners
| return if untraced_request?(request.env) | ||
| if untraced_request?(request.env) | ||
| ctx = OpenTelemetry::Common::Utilities.untraced | ||
| request.env[UNTRACED_CTX_TOKEN] = OpenTelemetry::Context.attach(ctx) |
There was a problem hiding this comment.
Why do we need another context (attached) and another token for this? We should add a context = OpenTelemetry::Context.current parameter to extract_remote_context and pass in the untraced context if necessary. I.e.:
parent_context = if untraced_request?(request.env)
extract_remote_context(request, OpenTelemetry::Common::Utilities.untraced)
else
extract_remote_context(request)
end
robertlaurin
force-pushed
the
rack-event-untraced
branch
from
a455e1c
to
d4fd8d5
Compare
robertlaurin
force-pushed
the
rack-event-untraced
branch
from
d4fd8d5
to
0d332ba
Compare
| @@ -52,9 +52,12 @@ class EventHandler | |||
| # @param [Rack::Response] This is nil in practice | |||
| # @return [void] | |||
| def on_start(request, _) | |||
| return if untraced_request?(request.env) | |||
| parent_context = if untraced_request?(request.env) | |||
There was a problem hiding this comment.
Is this relying on start_span to look for the untraced context as opposed to seething the current context to untraced without an active span?
There was a problem hiding this comment.
Yeah it's seeing that it's parent context is untraced so it will create a non recording span, and we have a lot of recording? checks in this instrumentation as is so it seemed like a simpler implementation than what I had in this commit a8548ef
There was a problem hiding this comment.
Understood. Thanks again for fixing this!
|
Bumping all the common deps here c8144bc |
Depends on common 0.21.0 for open-telemetry/opentelemetry-ruby#1634
closes: #962